Adding example for Identify protocol by dadepo · Pull Request #2689 · libp2p/rust-libp2p

dadepo · 2022-06-02T19:13:35Z

Description

Added example for the Identify protocol.

Links to any relevant issues

Part of #2684

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

mxinden

Appreciate the help here. Though still a couple of comments.

mxinden · 2022-06-03T15:28:09Z

examples/identify.rs

@@ -0,0 +1,82 @@
+// Copyright 2018 Parity Technologies (UK) Ltd.


Given that this example is focused on libp2p-identify, how about moving it to protocols/identify/examples/? To make sure one can still discover it, it can be mentioned in https://github.com/libp2p/rust-libp2p/blob/master/examples/README.md.

I was not even aware that there are some examples placed together with some of the individual protocols. Is there a reason why not to have all the examples listed in https://github.com/libp2p/rust-libp2p/tree/master/examples/*?

Having one canonical place where all the examples are listed, IMHO makes discovery easier.

No strong opinion on my end. I see /examples as a folder for examples that combine many protocols and protocols/xxx/examples as a folder for examples targeting a single protocol.

Having one canonical place where all the examples are listed, IMHO makes discovery easier.

I see your point.

Maybe @elenaf9 or @thomaseizinger have an opinion here.

I am leaning towards having examples focused on a single protocol in that protocol's folder. Since they are individual crates, imo they should "own" their own examples.

I was not even aware that there are some examples placed together with some of the individual protocols.

We should definitely mention this in the top-level examples/README.md, which is currently not the case.

I am leaning towards having examples focused on a single protocol in that protocol's folder. Since they are individual crates, imo they should "own" their own examples.

I share the same view. These protocols are individual crates and I think we should follow cargo conventions despite this being a large workspace.

I was not even aware that there are some examples placed together with some of the individual protocols.

We should definitely mention this in the top-level examples/README.md, which is currently not the case.

Another solution to this would be to do away with the current meta crate being "special" by being a workspace manifest and a crate at the same time. i.e. we could create a directory meta which could house the libp2p crate. This would also move the top-level examples/ directory which might make it more obvious to users that they will find examples within each crate.

mxinden · 2022-06-03T15:29:13Z

examples/identify.rs

+//! cargo run --example identify
+//! ```
+//! It will print the PeerId and the listening addresses, e.g. `Listening on
+//! "/ip4/0.0.0.0/tcp/24915"`


Suggested change

//! "/ip4/0.0.0.0/tcp/24915"`

//! "/ip6/2001:db8:: /tcp/24915"`

Let's not use a wildcard address here.

Given that we called swarm.listen_on with a ipv4 wildcard address, this can never result in a ipv6 listen address, right? Thus think "/ip4/127.0.0.1/tcp/24915" would make more sense here.

examples/identify.rs

Co-authored-by: Max Inden <mail@max-inden.de>

dadepo · 2022-06-03T19:13:48Z

Appreciate the help here. Though still a couple of comments.

Gladly done. I will resolve/respond the comments in a bit.

Quick question, seeing you already approved, do you prefer I create individual PR for each example added?
My initial thought was to include all the added examples in a single PR, hence why I set this PR to draft for now. Plan was to update it to ready PR once all the examples have been added

Let me know which approach the team finds more convenient.

mxinden · 2022-06-08T10:46:58Z

Quick question, seeing you already approved

That was a mistake. Sorry.

do you prefer I create individual PR for each example added?

Yes. I prefer small atomic pull requests. Makes my life as a maintainer a lot easier. Thanks.

mxinden · 2022-06-08T10:48:46Z

examples/identify.rs

@@ -0,0 +1,82 @@
+// Copyright 2018 Parity Technologies (UK) Ltd.


No strong opinion on my end. I see /examples as a folder for examples that combine many protocols and protocols/xxx/examples as a folder for examples targeting a single protocol.

Having one canonical place where all the examples are listed, IMHO makes discovery easier.

I see your point.

Maybe @elenaf9 or @thomaseizinger have an opinion here.

examples/identify.rs

Co-authored-by: Max Inden <mail@max-inden.de>

mxinden

🚀 Thank you!

CI failing

mxinden · 2022-06-24T03:30:10Z

CI failures are due to libp2p/if-watch#19.

mxinden · 2022-06-24T04:02:08Z

CI failures are due to mxinden/if-watch#19.

This is fixed now.

To make CI green you would need to:

Run rustfmt

Add libp2p as an explicit dev-dependency to protocols/identify/Cargo.toml similar to

rust-libp2p/protocols/dcutr/Cargo.toml

Line 33 in eb490c0

    
           libp2p = { path = "../..", default-features = false,  features = ["dcutr", "relay", "plaintext", "identify", "tcp-async-io", "ping", "noise", "dns-async-std"] }

elenaf9

Thanks @dadepo! Two comments, otherwise looks good to me.

elenaf9 · 2022-06-25T14:54:25Z

examples/identify.rs

+//! cargo run --example identify
+//! ```
+//! It will print the PeerId and the listening addresses, e.g. `Listening on
+//! "/ip4/0.0.0.0/tcp/24915"`


Given that we called swarm.listen_on with a ipv4 wildcard address, this can never result in a ipv6 listen address, right? Thus think "/ip4/127.0.0.1/tcp/24915" would make more sense here.

elenaf9 · 2022-06-25T14:55:52Z

protocols/identify/examples/identify.rs

+//! The dialing node prints out the `PeerId` of the node it is sending identify info to
+//! The other node prints out the received identify info.


I think not only the dialer, but instead both peers will sent each other identify info once they establish a connection, no?

Added identify example

9d41f5c

mxinden approved these changes Jun 3, 2022

View reviewed changes

dadepo changed the title ~~Added identify example~~ Adding more examples Jun 3, 2022

dadepo and others added 6 commits June 3, 2022 21:09

Update examples/identify.rs

04ca78f

Co-authored-by: Max Inden <mail@max-inden.de>

Update examples/identify.rs

1967874

Co-authored-by: Max Inden <mail@max-inden.de>

Update examples/identify.rs

0cfa33c

Co-authored-by: Max Inden <mail@max-inden.de>

Update examples/identify.rs

83c49a2

Co-authored-by: Max Inden <mail@max-inden.de>

Update examples/identify.rs

173e823

Co-authored-by: Max Inden <mail@max-inden.de>

Update examples/identify.rs

53360ca

Co-authored-by: Max Inden <mail@max-inden.de>

dadepo added 2 commits June 3, 2022 22:25

Start the comment with upper case

c4ae6e4

Merge branch 'master' into extend-examples

3d40130

mxinden reviewed Jun 8, 2022

View reviewed changes

dadepo and others added 2 commits June 23, 2022 18:06

Update examples/identify.rs

f60ad46

Co-authored-by: Max Inden <mail@max-inden.de>

Moved identify example to the identify crate

92188ad

dadepo changed the title ~~Adding more examples~~ Adding example for Identify protocol Jun 23, 2022

Merge branch 'master' into extend-examples

bbd783e

dadepo marked this pull request as ready for review June 23, 2022 16:23

dadepo requested review from elenaf9, mxinden and thomaseizinger June 23, 2022 16:24

mxinden previously approved these changes Jun 24, 2022

View reviewed changes

dadepo added 2 commits June 25, 2022 00:27

Ran rustfmt and added libp2p as an explicit dev-dependency

bd1e117

Merge branch 'master' into extend-examples

dbe73b7

elenaf9 reviewed Jun 25, 2022

View reviewed changes

Updated comments to be more accurate

f2668f3

elenaf9 approved these changes Jun 26, 2022

View reviewed changes

Merge branch 'master' into extend-examples

0d3dcd0

mxinden merged commit 40744be into libp2p:master Jun 27, 2022

dadepo deleted the extend-examples branch June 28, 2022 06:39

elenaf9 mentioned this pull request Jun 28, 2022

protocols/identify: fix dev dependencies required for examples/identify #2737

Merged

		@@ -0,0 +1,82 @@
		// Copyright 2018 Parity Technologies (UK) Ltd.

	//! "/ip4/0.0.0.0/tcp/24915"`
	//! "/ip6/2001:db8:: /tcp/24915"`

		//! The dialing node prints out the `PeerId` of the node it is sending identify info to
		//! The other node prints out the received identify info.

Conversation

dadepo commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links to any relevant issues

Change checklist

Uh oh!

mxinden left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elenaf9 Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dadepo commented Jun 3, 2022

Uh oh!

mxinden commented Jun 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Jun 24, 2022

Uh oh!

mxinden commented Jun 24, 2022

Uh oh!

elenaf9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

dadepo commented Jun 2, 2022 •

edited

Loading

mxinden left a comment •

edited

Loading

elenaf9 Jun 8, 2022 •

edited

Loading